Keep BYOD iDevice inventory managed-only - #53028
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved blocking issues were identified.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Updates iOS/iPadOS post-install refetches to request managed apps only for manually enrolled devices.
Changes:
- Applies enrollment-aware
ManagedAppsOnlybehavior. - Adds integration coverage for both enrollment types.
- Documents the user-visible fix.
File summaries
| File | Summary |
|---|---|
server/service/integration_vpp_install_test.go |
Tests inventory results for manual and automatic enrollment. |
server/service/apple_mdm_cmd_results.go |
Applies enrollment-aware app refetching. |
Review details
Files excluded by content exclusion policy (1)
- changes/52701-byod-idevice-managed-apps-only
- Files reviewed: 2/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. WalkthroughThe refetch-after-verification handler now reads the host MDM enrollment state and passes Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No concrete merge-blocking risk was established for this change. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #53028 +/- ##
==========================================
+ Coverage 76.03% 76.05% +0.01%
==========================================
Files 4120 4120
Lines 249738 249804 +66
Branches 14436 14436
==========================================
+ Hits 189896 189979 +83
+ Misses 59665 59648 -17
Partials 177 177
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| return ctxerr.Wrap(ctx, err, "request refetch for host after vpp install verification") | ||
| } | ||
| default: | ||
| hostMDM, err := ds.GetHostMDMCheckinInfo(ctx, installedAppResult.HostUUID()) |
There was a problem hiding this comment.
Would it maybe be better to use host_mdm.is_personal_enrollment rather than host_mdm.installed_from_dep here?
Example from claude:
--- a/server/service/apple_mdm_cmd_results.go
+++ b/server/service/apple_mdm_cmd_results.go
@@ -264,12 +264,14 @@
return ctxerr.Wrap(ctx, err, "request refetch for host after vpp install verification")
}
default:
- hostMDM, err := ds.GetHostMDMCheckinInfo(ctx, installedAppResult.HostUUID())
+ var hostMDM *fleet.HostMDM
+ hostMDM, err = ds.GetHostMDM(ctx, hostID)
if err != nil {
- return ctxerr.Wrap(ctx, err, "get host mdm info to refetch apps")
+ return ctxerr.Wrap(ctx, err, "get host mdm info to refetch apps")
}
- // BYOD devices are only queried for managed apps.
- isBYOD := !hostMDM.InstalledFromDEP
+ // Personal (BYOD) devices are only asked for managed apps.
+ managedAppsOnly := hostMDM.IsPersonalEnrollment
// Track before enqueueing so a fast device ack can't race the
// insert and leave an orphaned row; on enqueue failure nothing
@@ -275,7 +282,7 @@
return ctxerr.Wrap(ctx, err, "add host mdm commands")
}
- err = commander.InstalledApplicationList(ctx, []string{installedAppResult.HostUUID()}, fleet.RefetchAppsCommandUUID(), isBYOD)
+ err = commander.InstalledApplicationList(ctx, []string{installedAppResult.HostUUID()}, fleet.RefetchAppsCommandUUID(), managedAppsOnly)
if err != nil {
Related issue: Resolves #52701
The
InstalledApplicationListcommand Fleet sends after verifying an app install now asks for managed apps only on manually enrolled iPhones and iPads, matching the hourly refetch cron and the Refetch button. Before this the end user's own App Store apps landed in the host's software inventory until the next refetch removed them again.Keyed on
installed_from_depfor parity with the other two call sites. I think #52885 will want to move that predicate at all three together.Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
TestVPPInstallRefetchManagedAppsOnlyForBYODiDevicescovers both enrollment types. The simulated device reports a personal app only when the command omitsManagedAppsOnly, so the test asserts the resulting inventory and not just the flag. It fails onmainand passes here.Manual QA drove an in-house
.ipainstall through the MDM protocol with a simulated iPadOS device, against a clone of a dev database, on builds with and without the change:ManagedAppsOnlyfalseipa_test,PersonalGameipa_testtrueipa_testipa_testfalseipa_test,PersonalGameThe first row is the flip described in the issue.
AI
AI: Claude Code (claude-opus-5[1m])
Summary by CodeRabbit